-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add vJunos-Router kind based on existing vJunos-switch implementation #2063
Conversation
Adding the new node type to https://github.com/srl-labs/containerlab/blob/main/docs/manual/vrnetlab.md will require a new release to be carved both for vrnetlab and containerlab. |
Hi @vista- I wonder, could you try and add the new kind name to this list https://github.com/srl-labs/containerlab/blob/main/nodes/vr_vjunosswitch/vr-vjunosswitch.go#L20 kindnames = []string{"juniper_vjunosswitch","juniper_vjunosrouter"} and then remove the duplicated code? I suppose this should work nicely but I have no vrouter image to test this myself, so if you give it a spin that might be very helpful and should reduce the duplication |
@hellt merged the two kinds into one at your suggestion. For sake of simplicity, I left the vJunos-switch and vJunos-router's documentation separate. I also updated the documentation based on your feedback, pointing out that all vJunos images can be downloaded without a Juniper account. :) |
docs/index.md
Outdated
@@ -68,38 +69,38 @@ This short clip briefly demonstrates containerlab features and explains its purp | |||
|
|||
## Features | |||
|
|||
* **IaaC approach** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vista-
ah, you will have to bring those changes back =)
they are newlines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what got deleted here at the end of the line -- just two spaces at the end of the line? I don't think it was CRLF based on the line encodings in other files.
I honestly don't know what introduced these line ending changes (using VS code with default settings on Mac).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
microsoft/vscode#1679 (comment)
mystery solved 🤷
|
||
## Managing Juniper vJunosEvolved nodes | ||
|
||
!!!note | ||
Containers with vJunosEvolved inside will take ~15min to fully boot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will have to bring this back
docs/manual/kinds/vr-vjunosswitch.md
Outdated
|
||
## Managing Juniper vJunos-switch nodes | ||
|
||
!!!note | ||
Containers with vJunos-switch inside will take ~15min to fully boot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will have to bring this back =)
docs/manual/vrnetlab.md
Outdated
@@ -53,8 +53,8 @@ The following table provides a link between the version combinations: | |||
| `0.54.0` | [`0.16.0`](https://github.com/hellt/vrnetlab/releases/tag/v0.16.0) | Added support for Cisco c8000v | | |||
|
|||
???note "how to understand version inter-dependency between containerlab and vrnetlab?" | |||
When new VM-based platform support is added to vrnetlab, it is usually accompanied by a new containerlab version. In this case the table row will have both containerlab and vrnetlab versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will need those newlines added back
docs/manual/vrnetlab.md
Outdated
@@ -117,7 +118,7 @@ Containerlab offers several ways of connecting VM-based routers with the rest of | |||
<div class="mxgraph" style="max-width:100%;border:1px solid transparent;margin:0 auto; display:block;" data-mxgraph="{"page":6,"zoom":1.5,"highlight":"#0000ff","nav":true,"check-visible-state":true,"resize":true,"url":"https://raw.githubusercontent.com/srl-labs/containerlab/diagrams/vrnetlab.drawio"}"></div> | |||
|
|||
??? "Any other datapaths?" | |||
Although `tc` based datapath should cover all the needed connectivity requirements, if other bridge-like datapaths are needed, Containerlab offers OpenvSwitch and Linux bridge modes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will need those newlines added back
@@ -17,7 +17,7 @@ import ( | |||
) | |||
|
|||
var ( | |||
kindnames = []string{"juniper_vjunosevolved"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also are getting away from vr-
named kinds, so keep only juniper_vjunos...
in the kind names
schemas/clab.schema.json
Outdated
"vr-vjunosrouter": { | ||
"$ref": "#/definitions/node-config" | ||
}, | ||
"vr-juniper_vjunosrouter": { | ||
"$ref": "#/definitions/node-config" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when you remove the vr-
kind names from the go code we should also remove it from the schema
thanks @vista- thank you for your help! |
@hellt Just pushed some changes removing the vr-prefixed short names and fixing the newlines! Thanks for your quick review :) |
great, thank you! Merging |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2063 +/- ##
==========================================
- Coverage 53.82% 53.73% -0.10%
==========================================
Files 160 160
Lines 11524 11524
==========================================
- Hits 6203 6192 -11
- Misses 4458 4465 +7
- Partials 863 867 +4
|
This PR adds a vJunos-Router kind to match the recently added vrnetlab image: hellt/vrnetlab#196
Since both vJunos-Switch and vJunos-Router images are handled identically by containerlab, alternatively, the two kinds could be merged into a "vJunos" or "vJunos-Classic" kind that handles both vJunos-Switch and vJunos-Router nodes; however, this difference in naming might be confusing to users.
This PR also contains some minor fixes to syntactic fixes to the documentation and adds short name support for other vJunos kinds.